Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adjust incosistent pki log messages #8965

Merged
merged 1 commit into from
Nov 22, 2021
Merged

Adjust incosistent pki log messages #8965

merged 1 commit into from
Nov 22, 2021

Conversation

yhabteab
Copy link
Member

@yhabteab yhabteab commented Aug 9, 2021 •
edited
Loading

Checked all the error codes and couldn't find any others that could be raised,
even though the crt was issued by CA, apart from the revoked CRTs.

resolves #8682

@cla-bot cla-bot bot added the cla/signed label Aug 9, 2021
@icinga-probot icinga-probot bot added this to the 2.14.0 milestone Aug 9, 2021
@yhabteab yhabteab requested a review from Al2Klimov August 9, 2021 10:55
@yhabteab yhabteab force-pushed the bugfix/adjust-incosistent-pki-log-messages branch from 02561a2 to df8de02 Compare August 9, 2021 10:59
Al2Klimov
Al2Klimov requested changes Aug 9, 2021
View reviewed changes
lib/remote/jsonrpcconnection-pki.cpp Outdated Show resolved Hide resolved
Al2Klimov
Al2Klimov requested changes Aug 9, 2021
View reviewed changes
Copy link
Member

@Al2Klimov Al2Klimov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Write a test protocol.

@yhabteab
Copy link
Member Author

yhabteab commented Aug 9, 2021

Before:
See #8682 (comment)

After:

[2021-08-09 15:26:16 +0200] information/ApiListener: New client connection for identity 'satellite' from [::ffff:127.0.0.1]:55429 (certificate validation failed: code 23: certificate revoked)
[2021-08-09 15:26:16 +0200] information/JsonRpcConnection: Received certificate request for CN 'satellite' no longer trusted: certificate revoked (code 23)
[2021-08-09 15:26:16 +0200] information/JsonRpcConnection: Certificate request for CN 'satellite' is pending. Waiting for approval.
[2021-08-09 15:26:16 +0200] warning/JsonRpcConnection: API client disconnected for identity 'satellite'

@Al2Klimov
Copy link
Member

[2021-08-09 15:26:16 +0200] information/ApiListener: New client connection for identity 'satellite' from [::ffff:127.0.0.1]:55429 (certificate validation failed: code 23: certificate revoked)

And the other case?

@yhabteab
Copy link
Member Author

yhabteab commented Aug 9, 2021

[2021-08-09 15:26:16 +0200] information/ApiListener: New client connection for identity 'satellite' from [::ffff:127.0.0.1]:55429 (certificate validation failed: code 23: certificate revoked)

And the other case?

Which other case?

@yhabteab
Copy link
Member Author

yhabteab commented Aug 9, 2021

X509_V_ERR_CRL_HAS_EXPIRED ?

@Al2Klimov
Copy link
Member

Well, you've introduced an if with an else branch – two cases. One's already covered by #8965 (comment) – one not.

@yhabteab
Copy link
Member Author

yhabteab commented Aug 9, 2021 •
edited
Loading 

[2021-08-09 18:25:33 +0200] information/ApiListener: New client connection for identity 'satellite' from [::ffff:127.0.0.1]:56671 (certificate validation failed: code 23: certificate revoked)
[2021-08-09 18:25:33 +0200] information/JsonRpcConnection: Received certificate request for CN 'satellite' not signed by our CA: CRL has expired (code 12)
[2021-08-09 18:25:33 +0200] information/JsonRpcConnection: Certificate request for CN 'satellite' is pending. Waiting for approval.
[2021-08-09 18:25:33 +0200] warning/JsonRpcConnection: API client disconnected for identity 'satellite'

@yhabteab
Copy link
Member Author

yhabteab commented Aug 9, 2021

Actually, CRL has expired also falls under the no longer trusted flavor, doesn't it?

@Al2Klimov Al2Klimov requested a review from julianbrost August 9, 2021 16:31
@yhabteab yhabteab requested a review from julianbrost August 12, 2021 12:05
julianbrost
julianbrost requested changes Aug 13, 2021
View reviewed changes
Copy link
Contributor

@julianbrost julianbrost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My intention was not to give suggestions for the individual cases. What I wanted to say was to not attempt using different wording depending on the error cause but instead remove that if altogether and use a single message in all cases.

I just didn't want to give exact wording. You also have to ensure that it fits the rest of the message, for example "Received certificate request for CN 'foo' verification failed: [...]" sounds broken.

@yhabteab yhabteab force-pushed the bugfix/adjust-incosistent-pki-log-messages branch from c6edf2d to de33f5b Compare August 13, 2021 13:05
@yhabteab yhabteab requested a review from julianbrost October 22, 2021 14:53
julianbrost
julianbrost requested changes Nov 19, 2021
View reviewed changes
Copy link
Contributor

@julianbrost julianbrost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now the message is "Received certificate request for CN 'something' couldn't be verified: some error message", it should rather be "which couldn't be verified" to be correct.

@yhabteab yhabteab force-pushed the bugfix/adjust-incosistent-pki-log-messages branch from de33f5b to a8db3f0 Compare November 22, 2021 08:18
@cla-bot
Copy link

cla-bot bot commented Nov 22, 2021

Thank you for your pull request. Before we can look at it, you'll need to sign a Contributor License Agreement (CLA).

Please follow instructions at https://icinga.com/company/contributor-agreement to sign the CLA.

After that, please reply here with a comment and we'll verify.

Contributors that have not signed yet: @yhabteab

  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Please contact us if you think this is the case.

  • If you signed the CLA as a corporation, your GitHub username may not have been submitted to us. Please reach out to the responsible person in your organization.

@cla-bot cla-bot bot removed the cla/signed label Nov 22, 2021
@yhabteab
Copy link
Member Author

it should rather be "which couldn't be verified" to be correct.

Actually, I don't see any difference, but I have changed it the way you requested.

@julianbrost
Copy link
Contributor

it should rather be "which couldn't be verified" to be correct.

Actually, I don't see any difference, but I have changed it the way you requested.

On second thought, both versions are fine. Without "which", you have the read "received" as an adjective and not as the verb of the sentence (which I didn't in the first place).

@Al2Klimov
Copy link
Member

@cla-bot check

@cla-bot
Copy link

cla-bot bot commented Nov 22, 2021

Thank you for your pull request. Before we can look at it, you'll need to sign a Contributor License Agreement (CLA).

Please follow instructions at https://icinga.com/company/contributor-agreement to sign the CLA.

After that, please reply here with a comment and we'll verify.

Contributors that have not signed yet: @yhabteab

  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Please contact us if you think this is the case.

  • If you signed the CLA as a corporation, your GitHub username may not have been submitted to us. Please reach out to the responsible person in your organization.

@Al2Klimov
Copy link
Member

What's the problem here?

@julianbrost
Copy link
Contributor

The account was renamed and this change is probably not yet reflected in our CLA check.

@lippserd
Copy link
Member

@cla-bot check

@cla-bot cla-bot bot added the cla/signed label Nov 22, 2021
@Al2Klimov Al2Klimov merged commit 361807f into master Nov 22, 2021
@yhabteab yhabteab deleted the bugfix/adjust-incosistent-pki-log-messages branch November 22, 2021 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Al2Klimov Al2Klimov Al2Klimov requested changes

@julianbrost julianbrost julianbrost approved these changes

Assignees

@lippserd lippserd

Labels
cla/signed
Projects
None yet
Milestone
2.14.0
Development

Successfully merging this pull request may close these issues.

JSRPC-PKI: Logging incorrect error messages
4 participants
@yhabteab @Al2Klimov @julianbrost @lippserd